Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: fix missing fields, like scripts, in npm query when the virtual tree is loaded #5263

Merged
merged 2 commits into from Aug 8, 2022
Merged

fix: fix missing fields, like scripts, in npm query when the virtual tree is loaded #5263

merged 2 commits into from Aug 8, 2022
+66 −18

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Aug 4, 2022

arborist's loadActual() method checks for a hidden lockfile first, if one exists we load the tree virtually which causes us to lose quite a lot of data. this pull request introduces a new flag for arborist to forcibly skip this virtual loading and instead do an actual tree load, which brings back the properties we were missing

@nlf nlf requested a review from a team as a code owner August 4, 2022 20:56
@ljharb
Copy link
Collaborator

ljharb commented Aug 4, 2022

Is there an alternative, which is populating the hidden lockfile's virtual tree with all that on-disk data?

@nlf
Copy link
Contributor Author

nlf commented Aug 4, 2022

Is there an alternative, which is populating the hidden lockfile's virtual tree with all that on-disk data?

we could but then we have to carefully consider what data is currently available in the lockfile vs what users may want to query against, and we're likely to increase the size of the hidden lockfile by quite a bit.

to be honest, i'm pursuing some digging because i think the difference in available data when loading a tree virtually vs actually is very probably the source of some bugs. my first priority was fixing npm query so it will behave more consistently how users expect. it's entirely probable i'll be making some more foundational changes in arborist later, but that can wait.

@npm-cli-bot
Copy link
Collaborator

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 41.802 ±4.69 19.335 ±0.43 17.062 ±0.21 20.076 ±1.55 3.145 ±0.20 2.967 ±0.04 2.545 ±0.09 11.731 ±0.00 2.443 ±0.05 4.115 ±0.93
#5263 42.026 ±2.48 19.374 ±0.01 16.944 ±0.32 19.930 ±0.97 2.997 ±0.00 3.045 ±0.09 2.448 ±0.04 11.823 ±0.32 2.438 ±0.01 3.516 ±0.13
app-medium clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 34.861 ±8.10 14.580 ±0.01 12.606 ±0.07 13.461 ±0.49 2.686 ±0.05 2.707 ±0.00 2.442 ±0.05 8.751 ±0.08 2.301 ±0.03 3.174 ±0.09
#5263 27.695 ±0.76 14.796 ±0.15 12.978 ±0.15 13.903 ±0.26 2.790 ±0.06 2.745 ±0.09 2.452 ±0.02 8.753 ±0.04 2.323 ±0.00 3.119 ±0.01

@wraithgar wraithgar merged commit 9078e27 into latest Aug 8, 2022
@wraithgar wraithgar deleted the nlf/fix-npm-query-missing-fields branch August 8, 2022 15:46
@wraithgar wraithgar mentioned this pull request Aug 10, 2022
Merged
@npm-cli-bot npm-cli-bot mentioned this pull request Aug 10, 2022
Merged
@darcyclarke darcyclarke mentioned this pull request Aug 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wraithgar wraithgar wraithgar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nlf @ljharb @npm-cli-bot @wraithgar